docs(roles): state the limits of the operations allowlist - #631
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation in reference/users-and-roles/overview.md to clarify the scoping behavior and limitations of the operations field in permission objects. The review feedback suggests clarifying the explanation of how super_user roles interact with the allowlist to avoid misleading phrasing, and recommends using a VersionBadge component to properly document the behavior change introduced in version 5.0.0.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
harper#2176 moved gate one ahead of every other privilege check on the role, so the two limits this page originally described no longer hold: the structure_user carve-out and the SQL path both consulted the allowlist as of that change. Documents the 5.3.0 semantics instead — structure_user narrows which databases listed DDL reaches rather than widening which operations are reachable, and sql must be listed for a role to run SQL at all. The one limit that survives is that super_user/cluster_user roles cannot carry an allowlist: add_role and alter_role reject either flag set to true alongside other keys, so a 'restricted super user' is a validation error rather than a narrowed administrator. Drops the earlier claim about what authorization does with such a role, which described a state validation will not store and which #2176 inverted. Also notes that a non-array value can wedge the user-cache load (harper#2194), and adds the 5.3 release note, since the tightening revokes access from roles that relied on either bypass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
64c8bcb to
c4c697f
Compare
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
Matches the shape reference/database/compaction.md uses for the same kind of note: the badge on its own line, then a sentence naming what changed and an "earlier v5 releases" clause for the prior behavior. The badge was sitting mid-sentence immediately before a literal "Before v5.3.0", which rendered as "Changed in: v5.3.0 Before v5.3.0 ...". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
The three bullets explained gate ordering and which path bypassed what — the mechanism, not the task. A developer arriving here is writing a role and wants to know what to put in the list. Replaces them with one role that exercises all three limits (sql listed, DDL listed, structure_user scoping the DDL to one database) and two sentences of annotation. The super_user limit becomes the reason to build a role up rather than a bullet about validation. Same for the release note: what broke and the two cases to audit, rather than a narrative of the ordering change. Verified while rewriting that listing `sql` alone admits any statement variant on a direct SQL call — the allowlist checks `sql`, and the statement is then authorized against table permissions (harper DESIGN.md:2536), so the example can show SELECT/INSERT reaching only what the table grants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
dawsontoth
added
waiting-for-companion
|
Ready for review — but please do not merge this yet, even with an approval.
Please also do not arm auto-merge. Happy to flip it back to draft if that is a safer place to park it while it waits. Worth a careful look at two things:
🤖 Generated with Claude Code |
ldt1996
left a comment
ldt1996
left a comment
There was a problem hiding this comment.
Verified the substantive claims against the Harper source on current main rather than taking the description's word: verifyOperationsAllowlist exists with exactly the gate-one semantics documented here, its own comment confirms the SQL path must call it directly (serverUtilities.ts does, ahead of checkASTPermissions), api_name resolution matches the "written in snake_case" framing, and validateNoSUPerms plus OPERATIONS_MUST_BE_ARRAY back the role-shape statements. The example role is consistent with those rules, and the release note's two audit cases are the right upgrade caveats. Content-wise this is accurate and I would approve it.
Two process items:
-
The description says this is held as a draft until 5.3.0 is tagged, but the PR is currently marked ready for review. Only v5.3.0-alpha.1 exists as of today, and reference/ publishes as the live v5 site, so as it stands this is one merge click away from documenting unshipped behavior as current. Please flip it back to draft (or land whatever hold mechanism replaces it) until the stable tag exists; I am deliberately not approving while it is in a mergeable state its own body forbids.
-
Small wording nit, take or leave: "Gate one admits nothing the list omits" leans on internal gate numbering that the published page never introduces. Readers of the reference site have no gate one and gate two; "The allowlist admits nothing it omits, whatever else the role carries" says the same thing without the internal vocabulary.
Ping me when it is re-drafted or 5.3.0 tags and I will approve.
Lavinia, via Claude
Review nit from @ldt1996. The sentence read "Gate one admits nothing the list omits", which makes the reader carry the numbering from the two-gate list above into the rule itself. States the rule directly instead, and says where the check happens rather than which numbered gate it is — also matching the wording the 5.3 release note already uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for verifying against the source rather than the description — that is the check that matters most here, since the first version of this PR got two of these three claims backwards. On the wording nit (2): taken, with a note on the premise. The page does introduce the numbering — the paragraph above this one says "it acts as a two-gate check" and its second item says "for data operations that pass gate one" — so the vocabulary is not unestablished. But leaning on it in the sentence that states the rule still makes the reader carry a label forward to get the point, so I dropped it in
That also says where the check happens rather than which numbered gate it is, and matches the wording the 5.3 release note already uses. On the draft state (1): agreed on the substance, but that is not my call to reverse. Marking it ready was an explicit instruction from the PR author after I had flagged exactly the risk you are describing, so I have surfaced your request to them rather than flipping it back myself. You are right that the label and the body warning are a weaker gate than draft state was. Will ping you once that is resolved, or when 5.3.0 tags. 🤖 Generated with Claude Code |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
dawsontoth
removed
the
waiting-for-companion
|
@ldt1996 — withdrawing the hold, on the author's call. @dawsontoth: since the content is tagged The badge is what carries the disambiguation. A reader on 5.2.x sees "Changed in: v5.3.0" and knows the ordering described is not what their instance does, which is the same contract every other pre-tag 5.3 page on the site is already published under. That precedent is the part I had wrong when I asked you to hold it. So the bar I proposed was stricter than the one this repo actually applies, and holding this one PR to it would have made it the outlier. The description is updated accordingly, and the Your content verification and the gate-one wording fix both still stand — nothing about the substance changed. Over to you for approval whenever you are ready. To be explicit: I am not merging this myself, and auto-merge is not armed. 🤖 Generated with Claude Code |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Looks good, couple things to address.
🤖 Reviewed with Codex
|
|
||
| Operations normally restricted to `super_user` can be selectively granted by including them in the list. If `operations` is not set, the role can call any non-`super_user` operation, subject to table CRUD permissions. | ||
|
|
||
| <VersionBadge type="changed" version="v5.3.0" /> |
There was a problem hiding this comment.
The first affected release is v5.2.5, not v5.3.0. The tagged core sources establish that v5.2.4 lacks both the DDL/SQL allowlist enforcement and v5.2.5 contains them. Keeping this badge, with the corresponding warning at release-notes/v5-lincoln/5.3.md:35, leaves 5.2.4 -> 5.2.5 upgrades without an alert to list sql and DDL operations before access is revoked. Please badge this v5.2.5 and relocate the release-note warning to the 5.2.5 notes.
There was a problem hiding this comment.
Not seeing this — v5.2.4 and v5.2.5 are byte-identical on both files that would carry the enforcement:
$ git diff --stat v5.2.4 v5.2.5 -- utility/operation_authorization.ts \
server/serverHelpers/serverUtilities.ts
(no output)
And the enforcement is absent from the whole 5.2 line, not just 5.2.4. At v5.2.13:
verifyOperationsAllowlistdoes not exist at any 5.2.x tag (v5.2.3throughv5.2.13all absent).- The SQL path calls
sql.checkASTPermissions(json, parsedSqlObject)with no allowlist consultation —expandOperationsPermsappears zero times inserverUtilities.ts. - In
verifyPerms, thestructure_usercarve-out is still at line 604 and theoperationsgate at line 628, so DDL continues to bypass the list.
The change is harper#2176, merged to main 2026-08-21 and tagged only in v5.3.0-alpha.1. I checked for a back-port by file content rather than ancestry, since a cherry-pick would defeat git tag --contains — that is the failure mode your reading would fit, but the files show no cherry-pick happened.
So a 5.2.4 to 5.2.5 upgrade revokes nothing, and a warning in the 5.2.5 notes would tell operators to change roles for a tightening their release does not contain. Keeping the badge at v5.3.0. Happy to look again if you have a specific 5.2.5 behavior in mind that I am reading past.
🤖 Addressed by Claude Code
|
|
||
| <VersionBadge type="changed" version="v5.3.0" /> | ||
|
|
||
| An operation the list omits is denied whatever else the role carries — the list is checked ahead of every other permission on the role. Earlier v5 releases let table DDL and SQL around it; both now go through it. |
There was a problem hiding this comment.
structure_user schema operations remain an exception in MCP discovery: both visibility helpers advertise them whenever structure_user is truthy before consulting permission.operations. A role such as { structure_user: ['orders_db'], operations: ['sql'] } therefore lists create/drop MCP tools but rejects every invocation. That conflicts with this stated invariant and with reference/mcp/tools-and-resources.md:46-50, which promises that discovery shows only callable operations. Please fix the root cause in core by making both visibility helpers require the operation allowlist too, rather than documenting around the discrepancy.
There was a problem hiding this comment.
Confirmed, and it reproduces exactly as you describe. Both helpers short-circuit on structure_user truthiness before reaching the list:
// components/mcp/toolRegistry.ts:413 — canRoleInvokeOperation
if (perm.structure_user && SCHEMA_STRUCTURE_OPERATIONS.has(operation)) return true;
if (Array.isArray(perm.operations) && perm.operations.includes(operation)) return true;components/mcp/resources.ts:918 is the same two lines. SCHEMA_STRUCTURE_OPERATIONS holds all eight create/drop schema, table and attribute ops, and an array structure_user is truthy, so a role of { structure_user: ["orders_db"], operations: ["sql"] } advertises all eight and gate one denies all eight at dispatch.
One correction to the framing: this does not conflict with the invariant on this page. The sentence here is that an omitted operation is denied — which is what happens, as your own "rejects every invocation" confirms. canRoleInvokeOperation is a discovery filter, and its docblock is explicit that it is "intentionally conservative" and defers per-target checks to verifyPerms. What #2176 broke is that the deferral is no longer only about per-target predicates, so the filter is now wrong at the role level too.
The promise it does contradict is on the other page — reference/mcp/tools-and-resources.md says tools/list is filtered "so each session sees only the operations its user can actually call". Note the bullet just below it currently reads that a structure_user sees schema ops "in addition to anything in permission.operations", which documents the over-advertising as intended, so that page needs a pass either way once core changes.
On the fix itself: that is a core change in HarperFast/harper, outside what a docs PR can carry, so I am not making it here and have surfaced it to @dawsontoth rather than acting on it myself. Worth its own issue — the invocation side is already correct and fails closed, so this is a discovery-accuracy bug rather than an access one.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@kriszyp Would you consider this to block this PR?
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
This branch was previously deployed
3 participants
Documents the limits of a role's
permission.operationsallowlist onreference/users-and-roles/overview.md, plus the 5.3 release note for the tightening that produced them.Companion to HarperFast/studio#1628 (merged), which surfaces
permission.operationsin the Studio roles UI. Building that UI meant establishing the field's real semantics against the Harper source, and several of them contradict a natural reading of this page.Merge timing
Mergeable once approved. This documents behavior that is merged to
harpermainand tagged inv5.3.0-alpha.1, ahead of the stable 5.3.0 release, and that is the normal state for this repo rather than an exception:mainalready publishes a full 5.3 release-notes page and sevenreference/pages carrying<VersionBadge type="changed" version="v5.3.0" />(cli/commands.md,configuration/options.md,database/compaction.md,http/overview.md,operations-api/operations.md,security/certificate-verification.md,security/overview.md).The badge is what does the disambiguation — a reader on 5.2.x sees the changed-in marker and knows the ordering described here is not what their instance does. An earlier revision of this description asked reviewers to hold the merge until the stable tag; that was a stricter bar than the repo applies, and it is withdrawn.
Why this was rewritten
The original version of this PR described 5.2.x behavior. Three days after it was opened, harper#2176 moved allowlist gate one ahead of every other privilege check on the role and gave the SQL path its own call into it. That invalidated the two substantive limits the page had claimed:
structure_userbypasses the allowlist for DDLstructure_userthen scopes it to its databasessqlis authorized on its own path and never consults the allowlistsqlmust be listed for a role to run SQLThe third claim survives unchanged:
super_user/cluster_userroles cannot carry an allowlist, becausevalidateNoSUPermsrejects either flag set totruealongside other keys. The page's earlier sentence about what authorization then does with such a role is dropped — it described a state validation will not store, and #2176 inverted it anyway.What changes
reference/users-and-roles/overview.md— the three limits, with<VersionBadge type="changed" version="v5.3.0" />on the ordering change that produces two of them. Also states that the value must be an array, and points at harper#2194 for the non-array case, where a role carrying one can stop the instance loading its user cache.release-notes/v5-lincoln/5.3.md— a### Operation Allowlist Enforcementnote under## Security. The tightening revokes access from any role that relied on either bypass, so it owes an upgrade caveat: audit roles combiningoperationswithstructure_user, and roles whose list omitssql.Verification
Re-derived from the source at
v5.2.13andorigin/main(5.3.0-alpha.1), not from PR descriptions:utility/operation_authorization.ts—verifyOperationsAllowlist(new in #2176) runs after the empty-role check and before every privilege early-return; its own comment names the ordering requirement. Gate 2 (the SU-only-op grant) stays below the ambient privilege checks.server/serverHelpers/serverUtilities.ts:362— the SQL path callsverifyOperationsAllowlist(json, json.operation)directly, ahead ofcheckASTPermissions.validation/role_validation.ts—validateNoSUPermsgates onsuper_user === true/cluster_user === truewith more than one permission key (hence "set totrue" in the text;super_user: falsealongside an allowlist is fine).OPERATIONS_MUST_BE_ARRAYrejects a non-array. Unchanged betweenv5.2.13andmain.api_namemappings (CREATE_TABLE,DROP_DATABASE, …) are registered, so gate one resolves the camelCase handler names to the snake_case names an allowlist is written in.package.json: #2176 is merged,git tag --containsreturns onlyv5.3.0-alpha.1, androle_validation.ts/operationPermissions.tsconfirm the shippedv5.2.13behavior differs. Ships inv5.3.0.npm run format:checkclean;npm run buildsucceeds (407 documents), which also validates the new cross-link and the badge.Review coverage
Written by an LLM (originally Claude Fable 5; re-verified and rewritten by Claude Opus 5). Prose-only, two files. The underlying 5.2.x findings were reviewed by @cb1kenobi and @kriszyp on studio#1628; the 5.3.0 semantics here are newly derived and have not been reviewed by a human.
Both Gemini review comments are addressed — see the replies inline.
🤖 Generated with Claude Code